Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for line-number setting #90048

Merged
merged 2 commits into from
Oct 21, 2021

Conversation

GuillaumeGomez
Copy link
Member

The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test).

r? @jsha

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 19, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2021
Comment on lines 17 to 21
"margin": "0px",
"padding": "13px 8px",
"text-align": "right",
"border-top-left-radius": "5px",
"border-bottom-left-radius": "5px",
"font-size": "16px",
"font-family": '"Source Code Pro", monospace'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the test fragile. Any time we change these CSS properties, the test will break. I recommend choosing a thing or two that is semantically meaningful for the line numbers. For instance, text-align should always be right; if we miss that, things will look bad.

Also, can we check that the contents are a number? Maybe a specific number, like 1 for the first line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the <pre> contains all the lines at once separated by \n. But I can add a check. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the line numbers are a "hidden" feature (not enabled by default) and were not tested before this test (not great...). Considering it will be the only existing test for this feature, I think it makes sense to have one strict test to ensure it won't be forgotten and that any change that would impact it actually need to see why it broke.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking unrelated properties (like border radius) doesn't make a test better, it just makes it more fragile. Fragility causes a test to fail when someone changes something, but didn't actually break anything.

I think of tests like an alerting system: If they alert all the time, maintainers and contributors (a) get warning blindness, (b) start to disrespect the tests, and (c) start to mechanically update them whenever they fail, rather than looking at what actually caused them to break.

You don't want your tests to signal "something changed;" you want them to signal "something broke."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something changed in this case means that something broke. It also seems very unlikely that any of these properties changes unless you're working on code blocks. And in this case, you definitely want to be aware of this feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the border-radius though since changing it wouldn't be a big issue...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not everything that changes means something broke related to the line numbers. For instance: right now, the line numbers inherit their (calculated) font-size of 16em from a rule in normalize.css:

pre {
  font-size: 1em;
}

Imagine that changes to 2em. The will break a lot of our layouts, not just the line numbers. If that's something we're worried about (I don't think we are), we would want one test to fail: a test that checks that the contents of our <pre> tags are the right size. If instead a handful of tests about different features, like line numbers, failed, that would be very confusing and would result in the PR author thinking our tests are not meaningful. They might even give up their PR in frustration.

Another example: you're testing for the font face. That's set for all code and pre by this rule in rustdoc.css:

code, pre, a.test-arrow, .code-header {
  font-family: "Source Code Pro",monospace;
}

If we consciously decide to switch fonts for our pre tags in the future, we know we're changing fonts - we don't need a line-numbers unit test to tell us "this specific pre tag, out of all our pre tags, had a changed font."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I removed these checks.

@GuillaumeGomez
Copy link
Member Author

Added the text check.

@GuillaumeGomez GuillaumeGomez force-pushed the line-number-setting branch 2 times, most recently from 72319ed to fa5f08b Compare October 20, 2021 09:44
@rust-log-analyzer

This comment has been minimized.

@jsha
Copy link
Contributor

jsha commented Oct 20, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 20, 2021

📌 Commit 457f578 has been approved by jsha

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 20, 2021
… r=jsha

Add test for line-number setting

The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test).

r? `@jsha`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 20, 2021
… r=jsha

Add test for line-number setting

The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test).

r? ``@jsha``
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2021
… r=jsha

Add test for line-number setting

The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test).

r? ```@jsha```
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2021
Rollup of 14 pull requests

Successful merges:

 - rust-lang#86984 (Reject octal zeros in IPv4 addresses)
 - rust-lang#87440 (Remove unnecessary condition in Barrier::wait())
 - rust-lang#88644 (`AbstractConst` private fields)
 - rust-lang#89292 (Stabilize CString::from_vec_with_nul[_unchecked])
 - rust-lang#90010 (Avoid overflow in `VecDeque::with_capacity_in()`.)
 - rust-lang#90029 (Add test for debug logging during incremental compilation)
 - rust-lang#90031 (config: add the option to enable LLVM tests)
 - rust-lang#90048 (Add test for line-number setting)
 - rust-lang#90071 (Remove hir::map::blocks and use FnKind instead)
 - rust-lang#90074 (2229 migrations small cleanup)
 - rust-lang#90077 (Make `From` impls of NonZero integer const.)
 - rust-lang#90097 (Add test for duplicated sidebar entries for reexported macro)
 - rust-lang#90098 (Add test to ensure that the missing_doc_code_examples is not triggered on foreign trait implementations)
 - rust-lang#90099 (Fix MIRI UB in `Vec::swap_remove`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 47a1f67 into rust-lang:master Oct 21, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 21, 2021
@GuillaumeGomez GuillaumeGomez deleted the line-number-setting branch October 21, 2021 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants